Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for iterating only loaded values #311

Merged
merged 6 commits into from
Jul 11, 2023
Merged

Add support for iterating only loaded values #311

merged 6 commits into from
Jul 11, 2023

Conversation

fxamacker
Copy link
Member

Closes #303

Description

Added new functions and structs for a special iterators required by Stable Cadence:

  • Array.IterateLoadedValues()
  • ArrayLoadedValueIterator
  • OrderedMap.IterateLoadedValues()
  • MapLoadedValueIterator etc.

They have same API as regular iterators except that they only return already loaded elements.

  • Targeted PR against main branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@fxamacker
Add support for iterating only loaded values
d72d2fc 
Added new feature required by Stable Cadence.

New functions and structs include:
- Array.IterateLoadedValues()
- ArrayLoadedValueIterator
- OrderedMap.IterateLoadedValues()
- MapLoadedValueIterator
etc.

They have same API as regular iterators except that they only
return already loaded elements.
@fxamacker fxamacker added enhancement New feature or request E&V Team Execution / Verification / Edge Team labels Jun 23, 2023
@fxamacker fxamacker requested a review from SupunS June 23, 2023 21:44
@fxamacker fxamacker self-assigned this Jun 23, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2023 •
edited
Loading

Codecov Report

Merging #311 (646edb4) into main (a996413) will increase coverage by 0.45%.
The diff coverage is 73.64%.

@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
+ Coverage   64.55%   65.01%   +0.45%     
==========================================
  Files          14       14              
  Lines        8019     8387     +368     
==========================================
+ Hits         5177     5453     +276     
- Misses       2164     2236      +72     
- Partials      678      698      +20
Impacted Files Coverage Δ
storage.go 74.33% <45.45%> (-0.19%) ⬇️
storable.go 56.17% <61.90%> (+1.76%) ⬆️
map.go 67.20% <73.86%> (+0.49%) ⬆️
array.go 70.29% <77.37%> (+0.48%) ⬆️

turbolent
turbolent reviewed Jun 28, 2023
View reviewed changes
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

array.go Outdated Show resolved Hide resolved
@fxamacker
Merge branch 'main' into fxamacker/iterator-for-only-loaded-slabs
40866de
@fxamacker
Replace BFS with DFS for loaded value iterator
54e0b69 
Previously, Array and OrderedMap iterator used BFS to find all loaded
data slabs first and then iterated all loaded elements in those data slabs.

For Array and OrderedMap that are very large (with many data slabs),
the previous approach can be less efficient than necessary.

This PR uses DFS and tracks parents (slab iterators) in LIFO stack.
Iterator gets new data iterator from last parent in the stack.  Parent's
stack is reused and its length is maintained at max depth - 1.
@fxamacker fxamacker requested a review from turbolent June 29, 2023 17:49
turbolent
turbolent approved these changes Jun 30, 2023
View reviewed changes
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great work!

array.go Outdated Show resolved Hide resolved
map.go Outdated Show resolved Hide resolved
@turbolent
Copy link
Member

Maybe Supun (once he's back), Ramtin (once he's back), and/or someone else from the execution team can have a look as well before we merge

@turbolent
Copy link
Member

@SupunS could you please also have a look here?

@ramtinms
Copy link
Contributor

ramtinms commented Jul 5, 2023

I might need some context on this one.

  • Who would be the final user of this feature? cadence runtime code and or would it be directly exposed to the user's scripts?
  • How often the Atree internal cache of slabs is purged? I assume is at least once per transaction otherwise we might return values without reading them from the storage and cause missing register errors.

@fxamacker
Copy link
Member Author

@ramtinms Thanks for taking a look!

I might need some context on this one.
Who would be the final user of this feature? cadence runtime code and or would it be directly exposed to the user's scripts?

I think the plan is Cadence runtime. Bastian and Supun provided some context in reply to this comment on the issue.

How often the Atree internal cache of slabs is purged? I assume is at least once per transaction otherwise we might return values without reading them from the storage and cause missing register errors.

I think internal cache is purged on commit but I'm not sure how often commit is called. Maybe @turbolent or @janezpodhostnik can provide more info about how often that happens.

SupunS
SupunS approved these changes Jul 6, 2023
View reviewed changes
Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding this!

As usual, I don't know much about the atree internals, so mostly reviewed the general Go code. Looks great!

ramtinms
ramtinms approved these changes Jul 6, 2023
View reviewed changes
Copy link
Contributor

@ramtinms ramtinms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me,
I only recommend adding a small test that changes the map or array contents in the middle of an iteration. not sure this would happen ever but that's the only edge case that comes to my mind that didn't find it tests.

@turbolent turbolent mentioned this pull request Jul 6, 2023
Merged
6 tasks
janezpodhostnik
janezpodhostnik approved these changes Jul 11, 2023
View reviewed changes
Copy link

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me. I was especially looking for any sources of non-determinism, and it looks all good 👍.

@fxamacker
Add more tests for loaded value iterator
646edb4 
Added tests that remove slabs from storage during iteration
over loaded values.

The scenario tested shouldn't happen in practice but
we want to test that it would be handled gracefully.
@fxamacker
Merge branch 'main' into fxamacker/iterator-for-only-loaded-slabs
5db9320
@fxamacker fxamacker merged commit 24f8172 into main Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turbolent turbolent turbolent approved these changes

@janezpodhostnik janezpodhostnik janezpodhostnik approved these changes

@SupunS SupunS SupunS approved these changes

@ramtinms ramtinms ramtinms approved these changes

Assignees

@fxamacker fxamacker

Labels
E&V Team Execution / Verification / Edge Team enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Iteration over loaded values
6 participants
@fxamacker @codecov-commenter @turbolent @ramtinms @SupunS @janezpodhostnik